-
Notifications
You must be signed in to change notification settings - Fork 160
Use sol_get_sysvar instead of sol_get_<NAME>_sysvar, fixed #458
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
If this PR represents a change to the sysvar layout, please open a follow-up PR to update the JavaScript client |
|
If this PR represents a change to the sysvar layout, please open a follow-up PR to update the JavaScript client |
|
If this PR represents a change to the sysvar layout, please open a follow-up PR to update the JavaScript client |
|
If this PR represents a change to the sysvar layout, please open a follow-up PR to update the JavaScript client |
sysvar/src/epoch_rewards.rs
Outdated
| #[repr(C, packed)] | ||
| #[derive(Clone, Copy)] | ||
| struct EpochRewardsPacked { | ||
| distribution_starting_block_height: u64, | ||
| num_partitions: u64, | ||
| parent_blockhash: [u8; 32], | ||
| total_points: u128, | ||
| total_rewards: u64, | ||
| distributed_rewards: u64, | ||
| active: u8, // bool as u8 | ||
| } | ||
|
|
||
| const _: () = assert!(core::mem::size_of::<EpochRewardsPacked>() == 81); | ||
|
|
||
| impl From<EpochRewardsPacked> for EpochRewards { | ||
| fn from(p: EpochRewardsPacked) -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@febo please lmk what you think of this approach.
Originally I did a whole macro setup, but it was overkill given that we're only worried about 3 padded sysvars here.
Compile-time checks just below ensure 1) field parity between the two structs (repr(C) and packed) and 2) size
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a comment below about the use of packed.
| #[macro_export] | ||
| macro_rules! define_syscall { | ||
| (fn $name:ident($($arg:ident: $typ:ty),*) -> $ret:ty) => { | ||
| ($(#[$attr:meta])* fn $name:ident($($arg:ident: $typ:ty),*) -> $ret:ty) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enabling deprecated to be passed through (as in the original sol_get_sysvar PR)
|
If this PR represents a change to the sysvar layout, please open a follow-up PR to update the JavaScript client |
1 similar comment
|
If this PR represents a change to the sysvar layout, please open a follow-up PR to update the JavaScript client |
6c827fd to
a359358
Compare
|
If this PR represents a change to the sysvar layout, please open a follow-up PR to update the JavaScript client |
|
If this PR represents a change to the sysvar layout, please open a follow-up PR to update the JavaScript client |
sysvar/src/epoch_schedule.rs
Outdated
| #[repr(C, packed)] | ||
| #[derive(Clone, Copy)] | ||
| struct EpochSchedulePacked { | ||
| slots_per_epoch: u64, | ||
| leader_schedule_slot_offset: u64, | ||
| warmup: u8, // bool as u8 | ||
| first_normal_epoch: u64, | ||
| first_normal_slot: u64, | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using packed is problematic when there are unaligned fields – both first_normal_epoch and first_normal_slot are unaligned so can't be read directly.
What do you think if we change the definition of all sysvars to be like?
pub struct EpochSchedule {
/// The maximum number of slots in each epoch.
slots_per_epoch: [u8; 8],
/// A number of slots before beginning of an epoch to calculate
/// a leader schedule for that epoch.
leader_schedule_slot_offset: [u8; 8],
/// Whether epochs start short and grow.
warmup: u8,
/// The first epoch after the warmup period.
///
/// Basically: `log2(slots_per_epoch) - log2(MINIMUM_SLOTS_PER_EPOCH)`.
first_normal_epoch: [u8; 8],
/// The first slot after the warmup period.
///
/// Basically: `MINIMUM_SLOTS_PER_EPOCH * (2.pow(first_normal_epoch) - 1)`.
first_normal_slot: [u8; 8],
}
impl EpochSchedule {
pub fn slots_per_epoch(&self) -> u64 {
u64::from_le_bytes(self.slots_per_epoch)
}
pub fn leader_schedule_slot_offset(&self) -> u64 {
u64::from_le_bytes(self.leader_schedule_slot_offset)
}
// Could also return the `u8` value directly.
pub fn warmup(&self) -> Result<bool, ProgramError> {
match self.warmup {
0 => Ok(false),
1 => Ok(true),
_ => Err(ProgramError::InvalidAccountData),
}
}
// other fields...
pub fn first_normal_slot(&self) -> u64 {
u64::from_le_bytes(self.first_normal_slot)
}
}There is no "penalty" for using u64::from_le_bytes, although the implementation is a bit more verbose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, of course. The packed struct works currently since it’s just used for From, but that of course copies everything out.
I’ll implement your approach, thanks.
(Despite appearances here, I did in fact attend your optimization talk which mentioned this. exact. pattern. Ahem.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this constitute a breaking change (public fields removed)? I believe the internal-only packed version is better here - an unaligned read from a packed field produces the same IR/code as reading it as an array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@febo I've implemented your suggestion in this PR, and it's much nicer looking (and zero copy). However, as @jamie-osec mentions, the accessors are a breaking change. What formerly was e.g. .slots_per_epoch becomes .slots_per_epoch()
Note: I've only implemented it for the problematic Rent, EpochRewards, and EpochSchedule for now, pending our decision here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this constitute a breaking change (public fields removed)?
Yes, that is a drawback of the approach. We should be probably ok since crates are versioned individually.
I believe the internal-only packed version is better here - an unaligned read from a packed field produces the same IR/code as reading it as an array.
The issue is that we do an extra copy to return the "non-packed" version, which is what we would like to avoid. Returning the "packed" version is problematic – easy to end up in UB by taking a reference to an unaligned field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @joncinque
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this break would be annoying for Agave and some downstream programs that directly access any of these fields. We've introduced Pod* types for other sysvars, so I would recommend doing the same thing here
|
If this PR represents a change to the sysvar layout, please open a follow-up PR to update the JavaScript client |
2 similar comments
|
If this PR represents a change to the sysvar layout, please open a follow-up PR to update the JavaScript client |
|
If this PR represents a change to the sysvar layout, please open a follow-up PR to update the JavaScript client |
36610a1 to
d558879
Compare
|
If this PR represents a change to the sysvar layout, please open a follow-up PR to update the JavaScript client |
d558879 to
e1ef624
Compare
|
If this PR represents a change to the sysvar layout, please open a follow-up PR to update the JavaScript client |
|
If this PR represents a change to the sysvar layout, please open a follow-up PR to update the JavaScript client |
1 similar comment
|
If this PR represents a change to the sysvar layout, please open a follow-up PR to update the JavaScript client |
b2c4dd2 to
201ffb1
Compare
|
If this PR represents a change to the sysvar layout, please open a follow-up PR to update the JavaScript client |
|
If this PR represents a change to the sysvar layout, please open a follow-up PR to update the JavaScript client |
[WIP]
This addresses Issue #235, fixing two issues with the original (reverted) PR:
from_raw_parts_mutwas being violated, in a way that miri considered safe, but which could have theoretically caused trouble in the future.mem::size_of::<Self>(), which was not always true.Rent, for example, contains au64, anf64and au8. In memory the compiler pads this struct to 24 bytes, but the runtime stores its contents as a 17‑bytebincodeserialization. Theget_sysvarcall thus returns only 17 bytes. Passing a 24‑byte buffer results in anOFFSET_LENGTH_EXCEEDS_SYSVARerror and undefined behavior when uninitialized bytes are interpreted as f64.In order to address this second issue, this PR provides byte‑array representations of the sysvars with zero-copy accessors.
This is a breaking change since sysvar fields are now private, but updating is easy: dot fields are now dot accessors.
Note: this only updates the three sysvars
Rent,EpochSchedule, andEpochRewards. The others will follow in a subsequent PR.Thanks to jamie-osec for investigating the cause here and noticing the
from_raw_parts_mutviolation. I'll add you as contributor to the final.